Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-55454: Add IMAP4 IDLE support to imaplib #122542

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

foresto
Copy link

@foresto foresto commented Aug 1, 2024

This extends imaplib with support for the rfc2177 IMAP IDLE command, as requested in #55454. It allows events to be pushed to a client as they occur, rather than having to continually poll for mailbox changes.

The interface is a new idle() method, which returns an iterable context manager. Entering the context starts IDLE mode, during which events (untagged responses) can be retrieved using the iteration protocol. Exiting the context sends DONE to the server, ending IDLE mode.

An optional time limit for the IDLE session is supported, for use with servers that impose an inactivity timeout.

The context manager also offers a burst() method, designed for programs wishing to process events in batch rather than one at a time.

Notable differences from other implementations:

  • It's an extension to imaplib, rather than a replacement.
  • It doesn't introduce additional threads.
  • It doesn't impose new requirements on the use of imaplib's existing methods.
  • It passes the unit tests in CPython's test/test_imaplib.py module (and adds new ones).
  • It works on Windows, Linux, and other unix-like systems.
  • It makes IDLE available on all of imaplib's client variants (including IMAP4_stream).
  • The interface is pythonic and easy to use.

Caveats:

  • Due to a Windows limitation, the special case of IMAP4_stream running on Windows lacks a duration/timeout feature. (This is the stdin/stdout pipe connection variant; timeouts work fine for socket-based connections, even on Windows.) I have documented it where appropriate.

  • The file-like imaplib instance attributes are changed from buffered to unbuffered mode. This could potentially break any client code that uses those objects directly without expecting partial reads/writes. However, they are undocumented. As such, I think (and PEP 8 confirms) that they are fair game for changes. https://peps.python.org/pep-0008/#public-and-internal-interfaces

Usage examples:

#55454 (comment)

Recent discussion:

https://discuss.python.org/t/gauging-interest-in-my-imap4-idle-implementation-for-imaplib/59272

Earlier requests and suggestions:

#55454

https://mail.python.org/archives/list/[email protected]/thread/C4TVEYL5IBESQQPPS5GBR7WFBXCLQMZ2/


📚 Documentation preview 📚: https://cpython-previews--122542.org.readthedocs.build/

@foresto foresto requested a review from a team as a code owner August 1, 2024 03:13
Copy link

cpython-cla-bot bot commented Aug 1, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

This extends imaplib with support for the rfc2177 IMAP IDLE command,
as requested in python#55454.  It allows events to be pushed to a client as
they occur, rather than having to continually poll for mailbox changes.

The interface is a new idle() method, which returns an iterable context
manager.  Entering the context starts IDLE mode, during which events
(untagged responses) can be retrieved using the iteration protocol.
Exiting the context sends DONE to the server, ending IDLE mode.

An optional time limit for the IDLE session is supported, for use with
servers that impose an inactivity timeout.

The context manager also offers a burst() method, designed for programs
wishing to process events in batch rather than one at a time.

Notable differences from other implementations:

- It's an extension to imaplib, rather than a replacement.
- It doesn't introduce additional threads.
- It doesn't impose new requirements on the use of imaplib's existing methods.
- It passes the unit tests in CPython's test/test_imaplib.py module
  (and adds new ones).
- It works on Windows, Linux, and other unix-like systems.
- It makes IDLE available on all of imaplib's client variants
  (including IMAP4_stream).
- The interface is pythonic and easy to use.

Caveats:

- Due to a Windows limitation, the special case of IMAP4_stream running
  on Windows lacks a duration/timeout feature. (This is the stdin/stdout
  pipe connection variant; timeouts work fine for socket-based
  connections, even on Windows.) I have documented it where appropriate.

- The file-like imaplib instance attributes are changed from buffered to
  unbuffered mode. This could potentially break any client code that
  uses those objects directly without expecting partial reads/writes.
  However, these attributes are undocumented. As such, I think (and
  PEP 8 confirms) that they are fair game for changes.
  https://peps.python.org/pep-0008/#public-and-internal-interfaces

Usage examples:

python#55454 (comment)

Original discussion:

https://discuss.python.org/t/gauging-interest-in-my-imap4-idle-implementation-for-imaplib/59272

Earlier requests and suggestions:

python#55454

https://mail.python.org/archives/list/[email protected]/thread/C4TVEYL5IBESQQPPS5GBR7WFBXCLQMZ2/
@foresto
Copy link
Author

foresto commented Sep 1, 2024

Can someone find time for a code review?

Copy link
Contributor

@mcepl mcepl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I haven’t dived deep enough to the async stuff.

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
- Add example idle response tuples, to make the minor difference from other
  imaplib response tuples more obvious.
- Merge the idle context manager's burst() method docs with the IMAP
  object's idle() method docs, for easier understanding.
- Upgrade the Windows note regarding lack of pipe timeouts to a warning.
- Rephrase various things for clarity.
@foresto
Copy link
Author

foresto commented Sep 18, 2024

@mcepl Did you see my last comment? Have you had a chance to finish reviewing?

Copy link
Contributor

@mcepl mcepl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is very appropriate.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing, @foresto! With great PRs, comes great requests for changes 😉

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/test/test_imaplib.py Show resolved Hide resolved
Lib/imaplib.py Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/test/test_imaplib.py Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
gpshead and others added 5 commits September 21, 2024 10:39
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Peter Bierma <[email protected]>
@gpshead gpshead self-requested a review September 21, 2024 17:50
@ZeroIntensity
Copy link
Member

I think this also needs a "What's new in Python 3.14" entry.

foresto and others added 7 commits September 22, 2024 15:11
This is a correction to 8077f2e, which
changed a variable name in only one place and broke the subsequent
reference to it, departed from the naming convention used in the rest of
the module, and shadowed the type() builtin along the way.
This is for consistency with the documentation change in 8077f2e
and subsequent correction in 013bbf1.
foresto and others added 3 commits September 24, 2024 18:30
This reverts commit f385e44, because it
triggers CI failures in the docs by referencing a class that is
(deliberately) undocumented.
This is a different approach to f385e44, which was reverted for
creating dangling link references.

By prefixing the reStructuredText role target with a ! we disable
conversion to a link, thereby passing continuous integration checks
even though the referenced class is deliberately absent from the
documentation.
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ready for a core dev to review. (I haven't done a second pass, but everything I asked for in my initial review has been addressed.)

Thanks, @foresto!

@foresto
Copy link
Author

foresto commented Sep 26, 2024

@ZeroIntensity wrote:

I think this also needs a "What's new in Python 3.14" entry.

Is that not what this is?

@ZeroIntensity
Copy link
Member

It is, I wrote that comment before you added it (or maybe I missed it).

@foresto
Copy link
Author

foresto commented Sep 26, 2024

It is, I wrote that comment before you added it (or maybe I missed it).

Ah, gotcha. I thought for a moment that you meant some other place for What's New entries that I hadn't found yet. The bit I linked has been in this PR the whole time. :)

@ZeroIntensity
Copy link
Member

The bit I linked has been in this PR the whole time.

Ah, my bad. I have a tendency to get the what's new and news entries mixed up.

@foresto
Copy link
Author

foresto commented Oct 16, 2024

@gpshead are you still here?

Should I be looking for another core developer to move this forward?

@gpshead gpshead removed their request for review October 16, 2024 19:40
@gpshead
Copy link
Member

gpshead commented Oct 16, 2024

I won't have time to get to this one. Perhaps @encukou or @serhiy-storchaka could?

@ZeroIntensity
Copy link
Member

@malemburg was also the one that approved this on DPO, he might want a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants